fix(dbt): preserve aggregate expression semantics by eisber · Pull Request #437 · apache/ossie

eisber · 2026-09-21T09:24:54Z

Summary

Fixes the dbt converter's complex-expression fallback so it does not encode an already aggregated Ossie metric as both a MetricFlow expr and a second agg.

Decomposes a parser-proven root SQL aggregate into MetricFlow's aggregation operator and an aggregate-free scalar expression. For example, SUM(CASE WHEN ... THEN amount ELSE 0 END) becomes agg=sum plus expr=CASE WHEN ... THEN amount ELSE 0 END.
Removes the guessed-SUM fallback. Composite, nested, non-aggregate, unsupported-modifier, and non-SQL metric expressions are omitted with UNSUPPORTED_METRIC_EXPRESSION instead of silently changing semantics.
Surfaces Ossie-to-MSI conversion issues in the CLI and documents the supported behavior.

Before:

INPUT   SUM(CASE WHEN ... THEN amount ELSE 0 END)
MSI     agg=sum; expr=SUM(CASE WHEN ... THEN amount ELSE 0 END)
OUTPUT  SUM(SUM(CASE WHEN ... THEN amount ELSE 0 END))
issues  [] / []

After:

INPUT   SUM(CASE WHEN ... THEN amount ELSE 0 END)
MSI     agg=sum; expr=CASE WHEN ... THEN amount ELSE 0 END
OUTPUT  SUM(CASE WHEN ... THEN amount ELSE 0 END)
issues  [] / []

The invariant is that a MetricFlow SIMPLE metric has exactly one aggregation boundary: expr is scalar and agg supplies the aggregation.

Related Issues

Fixes #430

Checklist

Specification

No specification change is required; this corrects converter behavior against the existing field/metric expression distinction
Breaking changes are called out: unsupported fallback metrics are now reported and omitted rather than emitted with invented SUM semantics

Ontology

No ontology changes are required

Converters

dbt converter logic is updated
Positive, negative, CLI, and round-trip tests are included

Validation

No core validation change is required

Documentation

The dbt converter README documents the updated conversion behavior
No contribution-process change is required

Examples

No spec construct or example-model change is required

Tests

Full dbt suite passes: 112 tests, 5 snapshots
New functionality and unsupported cases are covered

Compliance

Existing ASF-licensed files only; no new source files
No third-party dependencies added

AI disclosure

Per the ASF Generative Tooling Guidance, this contribution was prepared with AI assistance. I reviewed the converter semantics, implementation, tests, and generated diff, and verified the results by running the full dbt suite locally.

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved expression-validation and SQL-dialect selection issues could produce incorrect metric conversions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Reject scalar expressions referencing multiple semantic models · New

What changed in this PR

Fixes dbt metric conversion to preserve aggregate semantics and report unsupported expressions.

Changes:

Decomposes supported aggregates into scalar expressions plus aggregation operators.
Reports and omits unsupported or non-SQL expressions.
Adds CLI warnings, tests, fixtures, and documentation.

File	Description
`converters/dbt/tests/test_ossie_to_msi.py`	Adds conversion, unsupported-case, and round-trip tests.
`converters/dbt/tests/test_cli.py`	Tests CLI warning output.
`converters/dbt/tests/helpers.py`	Supports dialect-specific test metrics.
`converters/dbt/src/ossie_dbt/ossie_to_msi.py`	Converts metrics and reports unsupported expressions.
`converters/dbt/src/ossie_dbt/expression_utils.py`	Parses aggregate expressions.
`converters/dbt/src/ossie_dbt/converter_issues.py`	Adds the unsupported-expression issue type.
`converters/dbt/src/ossie_dbt/cli.py`	Surfaces conversion warnings.
`converters/dbt/README.md`	Documents supported conversion behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    if isinstance(tree, exp.Sum) and _is_scalar_expression(tree.this):
        return AggregationType.SUM, _col_name(tree.this), None, False


jbonofre · 2026-09-21T13:14:59Z

+    return node.sql()
+
+
+def _is_scalar_expression(node: exp.Expression) -> bool:


This classifies "scalar" by blocklisting exp.AggFunc/exp.Window rather than allowlisting known-safe node types. Any aggregate sqlglot doesn't specifically model (LISTAGG, vendor/custom UDAFs) parses as generic exp.Anonymous and gets misclassified as scalar, which silently reintroduces the double-aggregation bug this PR is fixing.

Maybe also worth covering scalar subqueries (e.g. SUM((SELECT x FROM other_table LIMIT 1))) pass this check too and get embedded verbatim as expr.

I suggest inverting to an explicit allowlist of node types known to be safe as a scalar expression body, rather than trying to enumerate everything unsafe.

RyutoYoda · 2026-09-25T02:03:11Z

+                return dialect_expr
+        return ossie_expr.dialects[0] if ossie_expr.dialects else None
+
+    _SQL_DIALECTS = {


OSSIE_SQL_2026 is missing from this set, which turns a currently-working case into a dropped metric.

On main today, a metric whose expression is written only in OSSIE_SQL_2026 converts fine, because _get_expression() falls back to dialects[0]. I checked with the CLI:

metrics: - name: revenue expression: dialects: - dialect: OSSIE_SQL_2026 expression: SUM(orders.amount)

main: one metric, type_params.expr = amount. With this PR, _get_dialect_expression() returns that entry, dialect in self._SQL_DIALECTS is false, and the metric is dropped with UNSUPPORTED_METRIC_EXPRESSION.

The non-SQL dialects (MDX, MAQL, DAX, TABLEAU, SIGMA, THOUGHTSPOT) are rightly excluded, but OSSIE_SQL_2026 is the spec's own portable SQL, based on ANSI SQL:2003 Core (#439, #440), and expression_language.md asks every implementation to support the Ossie dialect. #443, #446 and #447 added it to the equivalent allowlists in orionbelt, databricks and snowflake for this exact reason, so dbt would be the one converter left out.

Suggested fix is one line:

Suggested change

_SQL_DIALECTS = {

_SQL_DIALECTS = {

OssieDialect.ANSI_SQL,

OssieDialect.OSSIE_SQL_2026,

OssieDialect.BIGQUERY,

OssieDialect.DATABRICKS,

OssieDialect.SNOWFLAKE,

}

Unrelated to this point, but worth flagging since it is the same function: #464 changes _get_expression() on main so that OSSIE_SQL_2026 is preferred over a vendor dialect rather than resolved by array position (#461). That PR and this one touch the same lines, so whichever lands second will need a rebase — happy for that to be mine.

fix(dbt): preserve aggregate expression semantics

1a444a1

Copilot AI lite review requested due to automatic review settings September 21, 2026 09:24

Copilot started reviewing on behalf of eisber September 21, 2026 09:25 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

Comment thread converters/dbt/src/ossie_dbt/expression_utils.py

Comment on lines +86 to 87

if isinstance(tree, exp.Sum) and _is_scalar_expression(tree.this):

return AggregationType.SUM, _col_name(tree.this), None, False

jbonofre self-requested a review September 21, 2026 12:21

jbonofre requested changes Sep 21, 2026

View reviewed changes

RyutoYoda mentioned this pull request Sep 25, 2026

fix(dbt): read OSSIE_SQL_2026 expressions #464

Merged

6 tasks done

RyutoYoda reviewed Sep 25, 2026

View reviewed changes

kayemkim mentioned this pull request Oct 2, 2026

fix(dbt): stop emitting COUNT(<dataset>.*), SUM(1) is the row count #495

Merged

6 tasks done

Navigation Menu

fix(dbt): preserve aggregate expression semantics - #437

fix(dbt): preserve aggregate expression semantics#437
eisber wants to merge 1 commit into
apache:mainfrom
eisber:dev/marcozo/develop-metric-aggregation-proposal-for-tlmnej

eisber commented Sep 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

jbonofre Sep 21, 2026

Uh oh!

RyutoYoda Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		if isinstance(tree, exp.Sum) and _is_scalar_expression(tree.this):
		return AggregationType.SUM, _col_name(tree.this), None, False

		return node.sql()


		def _is_scalar_expression(node: exp.Expression) -> bool:

-    _SQL_DIALECTS = {
+    _SQL_DIALECTS = {
+        OssieDialect.ANSI_SQL,
+        OssieDialect.OSSIE_SQL_2026,
+        OssieDialect.BIGQUERY,
+        OssieDialect.DATABRICKS,
+        OssieDialect.SNOWFLAKE,
+    }

Conversation

eisber commented Sep 21, 2026

Summary

Related Issues

Checklist

Specification

Ontology

Converters

Validation

Documentation

Examples

Tests

Compliance

AI disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

jbonofre Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

RyutoYoda Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants